Add: validation of bundled themes in build workflow #11627
Conversation
|
For the themes that currently fail to pass this we can comment out the offending keys and link to the issue. They're basically discarded while loading so commenting them out should be equivalent |
|
Updated the implementation to explicitly return validation failures rather than use log captures. This involved adding a new public factory constructor on the Please let me know if I've misunderstood what you were suggesting! EDIT: I see now that we need to update the loader in order to correctly handle inherited styles and palette colours. Will update |
|
I've updated the implementation to return both the theme and any validation failures from the The Let me know what you think @the-mikedavis |
helix-view/src/theme.rs
Outdated
| @@ -52,21 +52,24 @@ impl Loader { | |||
| } | |||
|
|
|||
| /// Loads a theme searching directories in priority order. | |||
| pub fn load(&self, name: &str) -> Result<Theme> { | |||
| pub fn load(&self, name: &str) -> Result<(Theme, Vec<String>)> { | |||
There was a problem hiding this comment.
Since we have many callers that need to handle the warnings let's create a load_with_warnings helper that has this function's body and then load can call that helper and log the warnings.
…ogging of theme warnings
| let (theme, warnings) = self.load_with_warnings(name)?; | ||
|
|
||
| for warning in warnings { | ||
| warn!("Theme '{}': {}", name, warning); |
There was a problem hiding this comment.
Logging theme name along with warning
|
Introduced helper method as suggested. Now that the warnings are being logged in the same place, I updated the warning messages to include the theme name to make it clearer to users which theme is the source of the warning |
* Add: xtask to check themes for validation warnings * Update: tidied up runtime paths * Update: test build workflow * Update: address clippy lints * Revert: only trigger workflow on push to master branch * Add: Theme::from_keys factory method to construct theme from Toml keys * Update: returning validation failures in Loader.load method * Update: commented out invalid keys from affected themes * Update: correct invalid keys so that valid styles still applied * Update: include default and base16_default themes in check * Update: renamed validation_failures to load_errors * Update: introduce load_with_warnings helper function and centralise logging of theme warnings * Update: use consistent naming throughout
Added xtask to validate bundled themes in order to detect issues at build time. Basically just loading all the themes are surfacing any warning logs which are emitted.
Main issue with the current implementation in the CI workflow is that it will cause builds on master to fail until the theme validation issues are addressed by their respective maintainers. Alternatively we could add a separate GitHub workflow which only triggers when the
runtime/themesdirectory is edited but this felt like overkill in this instance.Add: xtask for validating bundled themes
Update: build workflow to run theme validation on build
Closes #5709